cec: support for explicit control of Cilium Envoy filter injection#5
Open
MitchLewis930 wants to merge 1 commit intopr_045_beforefrom
Open
cec: support for explicit control of Cilium Envoy filter injection#5MitchLewis930 wants to merge 1 commit intopr_045_beforefrom
MitchLewis930 wants to merge 1 commit intopr_045_beforefrom
Conversation
Currently, the Cilium Envoy network- and L7 filters are always automatically injected when the CiliumEnvoyConfig is used for L7LB (parameter `isL7LB` - that is set to true when `Spec.Services` are defined on the CEC). This commit adds the possibility for a more explicit configuration of this behaviour by adding the annotation `cec.cilium.io/inject-cilium-filters`. If the annotation is present on the `CiliumEnvoyConfig` it is used to decide whether Cilium Envoy filters should be automatically injected or not. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for explicit control of Cilium Envoy filter injection in CiliumEnvoyConfig resources through a new annotation, decoupling filter injection from L7 load balancing configuration.
Changes:
- Introduces
cec.cilium.io/inject-cilium-filtersannotation to explicitly control filter injection - Refactors
parseResourcesto accept separateinjectCiliumEnvoyFiltersparameter instead of inferring fromisL7LB - Updates all test cases to accommodate the new parameter signature
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/annotation/k8s.go | Adds CECPrefix constant and CECInjectCiliumFilters annotation definition; reformats CiliumPrefixRegex variable declaration |
| pkg/ciliumenvoyconfig/cec_manager.go | Implements injectCiliumEnvoyFilters function and integrates it into resource parsing calls |
| pkg/ciliumenvoyconfig/cec_manager_test.go | Adds comprehensive test coverage for injectCiliumEnvoyFilters function and updates existing test calls |
| pkg/ciliumenvoyconfig/cec_resource_parser.go | Updates parseResources signature to accept injectCiliumEnvoyFilters parameter and refactors filter injection logic |
| pkg/ciliumenvoyconfig/cec_resource_parser_test.go | Updates all parseResources calls with new parameter and adds test for filter injection behavior |
| pkg/ciliumenvoyconfig/exp_reflector.go | Updates registerCECReflector to pass injectCiliumEnvoyFilters result to parseResources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cecObjectMeta.GetName(), | ||
| cecSpec.Resources, | ||
| len(cecSpec.Services) > 0, | ||
| len(cecSpec.Services) > 0, |
There was a problem hiding this comment.
This line should call injectCiliumEnvoyFilters(&cecObjectMeta, cecSpec) instead of duplicating the condition len(cecSpec.Services) > 0. The current code bypasses the annotation-based override logic and hardcodes the old behavior, making the new feature non-functional in the addCiliumEnvoyConfig path.
Suggested change
| len(cecSpec.Services) > 0, | |
| injectCiliumEnvoyFilters(&cecObjectMeta, cecSpec), |
| // added to all non-internal Listeners. In addition, the info gets passed to the Envoy Cilium BPF Metadata listener filter on all Listeners. | ||
| // - `isL7LB` defines whether these resources are used for L7 loadbalancing. If `true`, the info gets passed to | ||
| // the Envoy Cilium BPF Metadata listener filter on all Listeners. | ||
| // - `injecCiliumEnvoyFilters` defines whether the Envoy Cilium Network- and L7 filters should always be added to all non-internal Listeners. |
There was a problem hiding this comment.
Corrected spelling of 'injecCiliumEnvoyFilters' to 'injectCiliumEnvoyFilters'.
Suggested change
| // - `injecCiliumEnvoyFilters` defines whether the Envoy Cilium Network- and L7 filters should always be added to all non-internal Listeners. | |
| // - `injectCiliumEnvoyFilters` defines whether the Envoy Cilium Network- and L7 filters should always be added to all non-internal Listeners. |
Comment on lines
204
to
+205
|
|
||
| var ( | ||
| // CiliumPrefixRegex is a regex matching Cilium specific annotations. | ||
| CiliumPrefixRegex = regexp.MustCompile(`^([A-Za-z0-9]+\.)*cilium.io/`) | ||
| CECInjectCiliumFilters = CECPrefix + "/inject-cilium-filters" |
There was a problem hiding this comment.
The blank line at 204 creates inconsistent spacing in the constant declarations block. Consider removing it to maintain consistency with the surrounding code formatting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR_045